Skip to content

Implementation: #[feature(sync_nonpoison)], #[feature(nonpoison_mutex)] #144022

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 29, 2025

Conversation

connortsui20
Copy link
Contributor

@connortsui20 connortsui20 commented Jul 16, 2025

Continuation of #134663

Tracking Issue: #134645

This PR implements a new sync/nonpoison module, as well as the nonpoison variant of the Mutex lock.

There are 2 main changes here, the first is the new nonpoison::mutex module, and the second is the mutex integration tests.

For the nonpoison::mutex module, I did my best to align it with the current state of the poison::mutex module. This means that several unstable features (mapped_lock_guards, lock_value_accessors, and mutex_data_ptr) are also in the new nonpoison::mutex module, under their respective feature gates. Everything else in that file is under the correct feature gate (#[unstable(feature = "nonpoison_mutex", issue = "134645")]).

Everything in the nonpoison::mutex file is essentially identical in spirit, as we are simply removing the error case from the original poison::mutex.

The second big change is in the integration tests. I created a macro called that allows us to duplicate tests that are "generic" over the different mutex types, in that the poison mutex is always unwrapped.

I think that there is an argument against doing this, as it can make the tests a bit harder to understand (and language server capabilities are weaker within macros), but I think the benefit of code deduplication here is worth it. Note that it is definitely possible to generalize this (with a few tweaks) to testing the other nonpoison locks when they eventually get implemented, but I'll leave that for a later discussion.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 16, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@connortsui20 connortsui20 force-pushed the sync_nonpoison branch 2 times, most recently from e130930 to 804d305 Compare July 18, 2025 08:58
@connortsui20 connortsui20 marked this pull request as ready for review July 18, 2025 09:23
@rustbot
Copy link
Collaborator

rustbot commented Jul 18, 2025

r? @thomcc

rustbot has assigned @thomcc.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 18, 2025
@tgross35
Copy link
Contributor

r? tgross35

@rustbot rustbot assigned tgross35 and unassigned thomcc Jul 18, 2025
@connortsui20 connortsui20 force-pushed the sync_nonpoison branch 5 times, most recently from 35fc4df to 71395b1 Compare July 22, 2025 12:49
Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me, most comments here are related to docs. This will need some history cleanup, there isn't any need to preserve the current state of #134663 in history (I assume this was just done for now to show what changed since then)

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 23, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 23, 2025

This PR modifies tests/ui/issues/. If this PR is adding new tests to tests/ui/issues/,
please refrain from doing so, and instead add it to more descriptive subdirectories.

@bors
Copy link
Collaborator

bors commented Jul 26, 2025

☔ The latest upstream changes (presumably #144488) made this pull request unmergeable. Please resolve the merge conflicts.

@tgross35
Copy link
Contributor

Okay, this looks great! Thank you for getting this over the line, and @Aandreba for getting it started.

Could you squash "Update UI tests" into "add nonpoison::mutex implementation"? Its description doesn't really make sense on its own. "use generic function instead of macro" should ideally also be squashed somewhere. After that, r=me

connortsui20 and others added 5 commits July 29, 2025 10:32
Adds the equivalent `nonpoison` types to the `poison::mutex` module.
These types and implementations are gated under the `nonpoison_mutex`
feature gate.

Also blesses the ui tests that now have a name conflicts (because these
types no longer have unique names). The full path distinguishes the
different types.

Co-authored-by: Aandreba <[email protected]>
Co-authored-by: Trevor Gross <[email protected]>
This commit simply helps discern the actual changes needed to test both
poison and nonpoison locks.
Adds tests for the `nonpoison::Mutex` variant by using a macro to
duplicate the existing `poison` tests.

Note that all of the tests here are adapted from the existing `poison`
tests.
@tgross35
Copy link
Contributor

Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 29, 2025

📌 Commit d073d29 has been approved by tgross35

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 29, 2025
Zalathar added a commit to Zalathar/rust that referenced this pull request Jul 29, 2025
…oss35

Implementation: `#[feature(sync_nonpoison)]`, `#[feature(nonpoison_mutex)]`

Continuation of rust-lang#134663

Tracking Issue: rust-lang#134645

This PR implements a new `sync/nonpoison` module, as well as the `nonpoison` variant of the `Mutex` lock.

There are 2 main changes here, the first is the new `nonpoison::mutex` module, and the second is the `mutex` integration tests.

For the `nonpoison::mutex` module, I did my best to align it with the current state of the `poison::mutex` module. This means that several unstable features (`mapped_lock_guards`, `lock_value_accessors`, and `mutex_data_ptr`) are also in the new `nonpoison::mutex` module, under their respective feature gates. Everything else in that file is under the correct feature gate (`#[unstable(feature = "nonpoison_mutex", issue = "134645")]`).

Everything in the `nonpoison::mutex` file is essentially identical in spirit, as we are simply removing the error case from the original `poison::mutex`.

The second big change is in the integration tests. I created a macro called that allows us to duplicate tests that are "generic" over the different mutex types, in that the poison mutex is always `unwrap`ped.

~~I think that there is an argument against doing this, as it can make the tests a bit harder to understand (and language server capabilities are weaker within macros), but I think the benefit of code deduplication here is worth it. Note that it is definitely possible to generalize this (with a few tweaks) to testing the other `nonpoison` locks when they eventually get implemented, but I'll leave that for a later discussion.~~
bors added a commit that referenced this pull request Jul 29, 2025
Rollup of 14 pull requests

Successful merges:

 - #144022 (Implementation: `#[feature(sync_nonpoison)]`, `#[feature(nonpoison_mutex)]`)
 - #144167 (Document why `Range*<&T> as RangeBounds<T>` impls are not `T: ?Sized`, and give an alternative.)
 - #144407 (fix(debuginfo): disable overflow check for recursive non-enum types)
 - #144451 (fix: Reject upvar scrutinees for `loop_match`)
 - #144482 (Add explicit download methods to download module in bootstrap)
 - #144500 (thread name in stack overflow message)
 - #144511 (tidy: increase performance of auto extra checks feature)
 - #144586 (Update wasi-sdk to 27.0 in CI)
 - #144599 (bootstrap: enable tidy auto extra checks on tools profile)
 - #144600 (Ensure external paths passed via flags end up in rustdoc depinfo)
 - #144609 (feat: Right align line numbers)
 - #144623 (miri subtree update)
 - #144626 (cc dependencies: clarify comment)
 - #144627 (Add a test case for the issue #129882)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit that referenced this pull request Jul 29, 2025
Rollup of 13 pull requests

Successful merges:

 - #144022 (Implementation: `#[feature(sync_nonpoison)]`, `#[feature(nonpoison_mutex)]`)
 - #144167 (Document why `Range*<&T> as RangeBounds<T>` impls are not `T: ?Sized`, and give an alternative.)
 - #144407 (fix(debuginfo): disable overflow check for recursive non-enum types)
 - #144451 (fix: Reject upvar scrutinees for `loop_match`)
 - #144482 (Add explicit download methods to download module in bootstrap)
 - #144500 (thread name in stack overflow message)
 - #144511 (tidy: increase performance of auto extra checks feature)
 - #144599 (bootstrap: enable tidy auto extra checks on tools profile)
 - #144600 (Ensure external paths passed via flags end up in rustdoc depinfo)
 - #144609 (feat: Right align line numbers)
 - #144623 (miri subtree update)
 - #144626 (cc dependencies: clarify comment)
 - #144627 (Add a test case for the issue #129882)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4bc1b98 into rust-lang:master Jul 29, 2025
10 checks passed
@rustbot rustbot added this to the 1.90.0 milestone Jul 29, 2025
@bors
Copy link
Collaborator

bors commented Jul 29, 2025

⌛ Testing commit d073d29 with merge 7278554...

rust-timer added a commit that referenced this pull request Jul 29, 2025
Rollup merge of #144022 - connortsui20:sync_nonpoison, r=tgross35

Implementation: `#[feature(sync_nonpoison)]`, `#[feature(nonpoison_mutex)]`

Continuation of #134663

Tracking Issue: #134645

This PR implements a new `sync/nonpoison` module, as well as the `nonpoison` variant of the `Mutex` lock.

There are 2 main changes here, the first is the new `nonpoison::mutex` module, and the second is the `mutex` integration tests.

For the `nonpoison::mutex` module, I did my best to align it with the current state of the `poison::mutex` module. This means that several unstable features (`mapped_lock_guards`, `lock_value_accessors`, and `mutex_data_ptr`) are also in the new `nonpoison::mutex` module, under their respective feature gates. Everything else in that file is under the correct feature gate (`#[unstable(feature = "nonpoison_mutex", issue = "134645")]`).

Everything in the `nonpoison::mutex` file is essentially identical in spirit, as we are simply removing the error case from the original `poison::mutex`.

The second big change is in the integration tests. I created a macro called that allows us to duplicate tests that are "generic" over the different mutex types, in that the poison mutex is always `unwrap`ped.

~~I think that there is an argument against doing this, as it can make the tests a bit harder to understand (and language server capabilities are weaker within macros), but I think the benefit of code deduplication here is worth it. Note that it is definitely possible to generalize this (with a few tweaks) to testing the other `nonpoison` locks when they eventually get implemented, but I'll leave that for a later discussion.~~
@connortsui20 connortsui20 deleted the sync_nonpoison branch July 29, 2025 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants